Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add 'noop' transport #145

Merged
merged 2 commits into from
Aug 16, 2023
Merged

Add 'noop' transport #145

merged 2 commits into from
Aug 16, 2023

Conversation

subpop
Copy link
Collaborator

@subpop subpop commented Jul 21, 2023

Add a noop transport option. Setting the config flag 'protocol' to
"noop" or "" will configure yggd with a no-op transport. All the rest of
the dispatch and routing flows are unaffected, but no data is
transported to or from the client.

@subpop subpop requested review from jirihnidek and ahitacat July 21, 2023 19:19
@jirihnidek
Copy link
Contributor

jirihnidek commented Jul 24, 2023 •
edited
Loading

What is purpose of such functionality? It seems that yggd will be little bit useless in such situation. It looks like that yggd would be intended only as dispatcher of messages received over D-Bus, right? Isn't yggd overkill in such situation? We can easily send D-Bus methods and signal on localhost without anything like yggdrasil?

I can call echo worker, when yggd is not running like this:

busctl --user call com.redhat.Yggdrasil1.Worker1.echo /com/redhat/Yggdrasil1/Worker1/echo \
    com.redhat.Yggdrasil1.Worker1 Dispatch sssa{ss}ay "echo" "baa129a7-e4d6-453d-9380-f89a69c5f192" \
    "34176e48-11c4-422d-bfc7-5a07cbb0b798" "0" "3" 0x01 0x02 0x03

@subpop
Copy link
Collaborator Author

subpop commented Jul 24, 2023

Yes, it does convert yggd into little more than a wrapper around D-Bus. I don't have an intended use for this change aside from making my life a little easier as a developer. Being able to turn off the MQTT transport requirements make my ability to develop the other parts of yggd a little faster. And who knows? Maybe a customer would find some bizarre use for this. Temporarily stopping transport during maintenance windows, perhaps?

jirihnidek
jirihnidek requested changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have few comments and suggestions.

cmd/yggd/main.go Outdated
@@ -140,6 +140,12 @@ func setupClient(
if err != nil {
return nil, nil, cli.Exit(fmt.Errorf("cannot create HTTP transport: %w", err), 1)
}
case "", "noop":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use only "noop". I would not use "" as another option for this "protocol". I will explain why.

Common practice is following. When "" is used for value of some option, then default option is used (default protocol is "mqtt"). Thus suggested behavior would be confusing. The "noop" could be used in /etc/yggdrasil/config.toml for option protocol, right?

BTW: Is the "noop" the best keyword? I think that it is not so well known word. I tried to find it in some dictionaries and most dictionaries does not contain this word. What about "none" keyword for this purpose? If the "noop" was used, then I would add some description to man page. The problem is that we generate man page. Adding additional information to man page would probably require adding custom function for generating man page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Yes. I thought about whether I should include "" or not. You make a good point. I'll drop it from the case condition. I went around and around on a name, including LoopTransport, NullTransport and a few others. Neither name really fit the functionality of the transport until I settled on the term "no-op". It's an abbreviation for "no operation". I felt this name fit the functionality of the transport quite well, as it doesn't loop anything back; it just takes no operation on the data. I like the term "none" to describe the protocol. Any objections against?

Suggested change
case "", "noop":
case "none", "noop":

internal/transport/noop.go Show resolved Hide resolved
@jirihnidek
Copy link
Contributor

Yes, it does convert yggd into little more than a wrapper around D-Bus. I don't have an intended use for this change aside from making my life a little easier as a developer. Being able to turn off the MQTT transport requirements make my ability to develop the other parts of yggd a little faster. And who knows? Maybe a customer would find some bizarre use for this. Temporarily stopping transport during maintenance windows, perhaps?

OK. It makes sense.

@subpop subpop force-pushed the optional-network-transport branch from 10b65ee to 8aaa72b Compare July 25, 2023 15:14
@subpop subpop requested a review from jirihnidek July 26, 2023 16:45
@subpop subpop force-pushed the optional-network-transport branch 2 times, most recently from 527518c to 235153b Compare August 3, 2023 16:14
jirihnidek
jirihnidek requested changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use "none" or "noop". Not both. It could be confusing to see "noop" and "none" for the same thing.

cmd/yggd/main.go Show resolved Hide resolved
cmd/yggd/main.go Outdated Show resolved Hide resolved
@subpop subpop force-pushed the optional-network-transport branch from 235153b to ae7be8a Compare August 7, 2023 16:54
@subpop subpop requested a review from jirihnidek August 7, 2023 16:55
jirihnidek
jirihnidek requested changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I promise that have final request/comment. :-)

cmd/yggd/main.go Outdated Show resolved Hide resolved
@subpop subpop force-pushed the optional-network-transport branch from ae7be8a to 90d3541 Compare August 15, 2023 16:12
@subpop
feat: add 'noop' transport
bd0b192 
Add a noop transport option. Setting the config flag 'protocol' to
"none" or "" will configure yggd with a no-op transport. All the rest of
the dispatch and routing flows are unaffected, but no data is
transported to or from the client.

Signed-off-by: Link Dupont <link@sub-pop.net>
@subpop subpop force-pushed the optional-network-transport branch from 90d3541 to bd0b192 Compare August 15, 2023 16:13
@subpop subpop requested a review from jirihnidek August 15, 2023 17:33
jirihnidek
jirihnidek approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍 Thanks for updates.

@jirihnidek jirihnidek merged commit dddbfa9 into main Aug 16, 2023
@jirihnidek jirihnidek deleted the optional-network-transport branch August 16, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jirihnidek jirihnidek jirihnidek approved these changes

@ahitacat ahitacat Awaiting requested review from ahitacat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@subpop @jirihnidek